Skip to content

Show warning message if initial_parameters is passed instead of initial_params - #176

Merged
shravanngoswamii merged 8 commits into
mainfrom
sg/#175
Oct 2, 2025
Merged

Show warning message if initial_parameters is passed instead of initial_params#176
shravanngoswamii merged 8 commits into
mainfrom
sg/#175

Conversation

@shravanngoswamii

@shravanngoswamii shravanngoswamii commented Oct 1, 2025

Copy link
Copy Markdown
Member

Closes #175

@shravanngoswamii
shravanngoswamii requested a review from yebai October 1, 2025 15:11
@github-actions

github-actions Bot commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

AbstractMCMC.jl documentation for PR #176 is available at:
https://TuringLang.github.io/AbstractMCMC.jl/previews/PR176/

@TuringLang TuringLang deleted a comment from github-actions Bot Oct 1, 2025
yebai
yebai approved these changes Oct 1, 2025
View reviewed changes

@yebai yebai left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @shravanngoswamii!

@yebai
yebai requested a review from penelopeysm October 1, 2025 21:10
penelopeysm
penelopeysm suggested changes Oct 1, 2025
View reviewed changes

@penelopeysm penelopeysm left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @shravanngoswamii -- there is one thing missing here, namely that single-chain sampling doesn't issue a warning.

using AbstractMCMC
struct M <: AbstractMCMC.AbstractModel end
struct S <: AbstractMCMC.AbstractSampler end
AbstractMCMC.step(rng, ::M, ::S, state=nothing; kwargs...) = rand(rng), nothing
sample(M(), S(), 5; initial_parameters=42.42)

Now if you were to copy-paste the warning inside the method for mcmcsample, my guess is that you will probably find that multiple-chain sampling

sample(M(), S(), MCMCThreads(), 10, 3; initial_parameters=42.42)

will issue a warning (nchains+1) times, because the keyword arguments are passed straight on to single-chain sample.

So, if I'm not wrong, in the multiple-chain sampling when you are performing this check you should also make sure that initial_parameters is removed from kwargs after issuing the warning, e.g. using:

kwargs = BangBang.delete!!(kwargs, :initial_parameters)

(I hunted around for a base Julia way of removing a key from a NamedTuple, but it's not released yet: JuliaLang/julia#55270 so I think BangBang is needed.)

@shravanngoswamii

shravanngoswamii commented Oct 2, 2025

Copy link
Copy Markdown
Member Author

Hi @penelopeysm, the kwargs are of type Base.Pairs, and I think BangBang.delete!! doesn’t work with Base.Pairs (from kwargs...). I tried it, but it didn’t work, so I used manual filtering instead. Maybe I’m wrong, though—I haven’t worked with these things as much.

Also, I noticed something in the DynamicPPL PR that @yebai mentioned in #175. You used hasproperty(kwargs, :initial_parameters), but I think hasproperty doesn’t work with Base.Pairs (what kwargs... produces). You should use haskey instead. Additionally, test coverage seems to be missing for this warning (Codecov shows "2 Missing" lines), so maybe that wasn't detected.

@shravanngoswamii shravanngoswamii changed the title Show warning message if initial_parameters is passed instead of initial_params Show warning message if initial_parameters is passed instead of initial_params Oct 2, 2025
@penelopeysm

penelopeysm commented Oct 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Oh, yes, good catch. Sure, the DynamicPPL bit has similar problems to what I described here too (including with removing the keyword argument). But if the warning is in AbstractMCMC, then there's no need for the DynamicPPL one, and I can just remove it.

@shravanngoswamii

shravanngoswamii commented Oct 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

But if the warning is in AbstractMCMC, then there's no need for the DynamicPPL one, and I can just remove it.

Okay, let's merge it!

@penelopeysm

penelopeysm commented Oct 2, 2025

Copy link
Copy Markdown
Contributor

Will merge when CI passes, thanks @shravanngoswamii!

yebai
yebai reviewed Oct 2, 2025
View reviewed changes
Comment thread src/sample.jl Outdated
@shravanngoswamii
shravanngoswamii merged commit ae9760d into main Oct 2, 2025
18 checks passed
@shravanngoswamii
shravanngoswamii deleted the sg/#175 branch October 2, 2025 18:32
@codecov

codecov Bot commented Oct 2, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (fc74dd4) to head (539d06b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@     Coverage Diff     @@
##   main   #176   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shravanngoswamii shravanngoswamii added the hacktoberfest-accepted https://hacktoberfest.com/ label Oct 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yebai yebai yebai approved these changes
+1 more reviewer
@penelopeysm penelopeysm penelopeysm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@shravanngoswamii shravanngoswamii

Labels

hacktoberfest-accepted https://hacktoberfest.com/

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Show warning message if initial_parameters is passed instead of initial_params

3 participants

@shravanngoswamii @penelopeysm @yebai